Feature/download in stream mode #755

9547 · 2020-09-03T17:24:37Z

What problem does this PR solve?

#728

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Code changes

Has exported function/method change
Has exported variable/fields change
Has interface methods change
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

The original tar.gz file is saved, not deleted, delete it or not is a question. Currently the disk layout as below:

eg: tispark

(base) root@rabbit:tiup <feature/download-in-stream-mode(v1.1.0-18-gfe49c91) ✖️  ># tree ~/.tiup/components/tispark/
/home/.tiup/components/tispark/
└── v2.3.1
    ├── tispark-assembly-2.3.1.jar
    └── tispark-v2.3.1-any-any.tar.gz ## not deleted

Release notes:

NONE

9547 · 2020-09-03T17:28:36Z

@lonng PTAL. BTW, is there any way to run GitHub CI Actions locally, or should we write some scripts to run the e2e test locally?

lonng · 2020-09-04T01:30:37Z

@9547 Yes, you can run the integration locally. e.g:

workdir=$(pwd)
cd $(workdir)/docker
TIUP_CLUSTER_ROOT=$workdir ./up.sh --daemon --dev --compose ./docker-compose.dm.yml
docker exec  tiup-cluster-control bash /tiup-cluster/tests/tiup-dm/run.sh --native-ssh --do-cases test_upgrade

codecov-commenter · 2020-09-13T12:10:56Z

Codecov Report

Merging #755 into master will increase coverage by 0.11%.
The diff coverage is 50.94%.

@@            Coverage Diff             @@
##           master     #755      +/-   ##
==========================================
+ Coverage   52.39%   52.50%   +0.11%     
==========================================
  Files         262      262              
  Lines       18840    18876      +36     
==========================================
+ Hits         9871     9911      +40     
+ Misses       7432     7414      -18     
- Partials     1537     1551      +14

Flag	Coverage Δ
#cluster	`44.79% <16.98%> (+0.07%)`	⬆️
#dm	`25.34% <16.98%> (+0.06%)`	⬆️
#integrate	`47.69% <39.62%> (+0.07%)`	⬆️
#playground	`21.91% <32.00%> (+0.22%)`	⬆️
#tiup	`10.93% <33.96%> (+0.19%)`	⬆️
#unittest	`18.51% <46.00%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/environment/env.go	`31.92% <0.00%> (-0.39%)`	⬇️
pkg/cluster/clusterutil/cluster.go	`61.29% <33.33%> (-0.88%)`	⬇️
pkg/repository/mirror.go	`48.76% <42.85%> (+7.09%)`	⬆️
pkg/repository/v1manifest/local_manifests.go	`38.78% <50.00%> (-0.61%)`	⬇️
pkg/repository/v1_repository.go	`65.38% <61.29%> (-0.74%)`	⬇️
pkg/cluster/api/pdapi.go	`54.45% <0.00%> (+3.30%)`	⬆️
pkg/utils/ioutil.go	`51.32% <0.00%> (+5.30%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 800fc9f...0f329de. Read the comment docs.

lucklove · 2020-09-16T04:12:35Z

@9547 Do you have any idea about the failed test? Or do you need help?

pkg/repository/v1_repository.go

lucklove · 2020-09-16T07:45:20Z

Your tests failed because we didn't call mirror.Open after NewMirror, it's not your problem. Just add it after this line and it should work

9547 · 2020-09-16T23:14:11Z

Your tests failed because we didn't call mirror.Open after NewMirror, it's not your problem. Just add it after this line and it should work

check and fixed

9547 · 2020-09-16T23:32:44Z

@lucklove Seems the test case failed by two reasons:

failed to fetch components, such as tispark, maybe caused by CDN.

some components failed to startup, such as below:

Starting component tikv
    Starting instance tikv 172.19.0.105:20160
    Starting instance tikv 172.19.0.102:20160
    Starting instance tikv 172.19.0.103:20160
    Starting instance tikv 172.19.0.104:20160
    Start tikv 172.19.0.102:20160 success
    Start tikv 172.19.0.105:20160 success
    Start tikv 172.19.0.104:20160 success
retry error: operation timed out after 2m0s
    tikv 172.19.0.103:20160 failed to start: timed out waiting for port 20160 to be started after 2m0s, please check the log of the instance

Error: failed to start tikv: 	tikv 172.19.0.103:20160 failed to start: timed out waiting for port 20160 to be started after 2m0s, please check the log of the instance: timed out waiting for port 20160 to be started after 2m0s

1/4 tikv instancts failed to startup. 🤔

lucklove · 2020-09-17T04:10:35Z

@9547 I rerun the tests and the only failure is that local package can't be found. This is because you didn't move the downloaded package to correct path/name after r.mirror.Download returned. eg. the url of the nightly of tidb will be /tidb-v5.0.0-nightly-20200917-linux-amd64.tar.gz, so after downloading it will be saved as ~/.tiup/storage/cluster/packages/tidb-v5.0.0-nightly-20200917-linux-amd64.tar.gz. However, the target is given as ~/.tiup/storage/cluster/packages/tidb-v5.0.0-nightly-linux-amd64.tar.gz. So you should check if path.Join(targetDir, item.URL) equals to target, if not, you should apply a move action.

9547 · 2020-09-17T15:19:26Z

/run-all-tests

9547 · 2020-09-17T22:54:35Z

@lucklove Sorry, a new test case failed, but running it locally is fine. This test also failed strangely, the log said Process completed with exit code 124., IMO it maybe casued by curl https://tiup-mirrors.pingcap.com/root.json -o $TMP_DIR/home/bin/root.json timed out, right? But which case was also used in integrate-tiup and no failure. thanks to your help.

5m 6s
    GOROOT: /opt/hostedtoolcache/go/1.14.9/x64
Run export PATH=$PATH:/home/runner/work/tiup/tiup/go/src/github.com/pingcap/tiup/bin/
  export PATH=$PATH:/home/runner/work/tiup/tiup/go/src/github.com/pingcap/tiup/bin/
  bash /home/runner/work/tiup/tiup/go/src/github.com/pingcap/tiup/tests/tiup-playground/test_playground.sh
  shell: /bin/bash -e {0}
  env:
    working-directory: /home/runner/work/tiup/tiup/go/src/github.com/pingcap/tiup
    GOROOT: /opt/hostedtoolcache/go/1.14.9/x64
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  7275  100  7275    0     0  16609      0 --:--:-- --:--:-- --:--:-- 16609
##[error]Process completed with exit code 124.

lucklove · 2020-09-18T02:24:35Z

@9547 Yes, I agree with you. It may be the problem of curl https://tiup-mirrors.pingcap.com/root.json. But I don't know the reason yet.

lucklove · 2020-09-18T02:53:41Z

@9547 catch it. You can replay this locally:

rm -rf ~/.tiup/components/*  # delete installed components first, in case it will use installed and skip downloading
tiup install playground:v1.1.2
make playground
cp bin/tiup-playground ~/.tiup/components/playground/v1.1.2/
tiup playground

It's also the mirror problem... We forget open the mirror in the function InitEnv

9547 · 2020-09-18T03:36:14Z

Thanks for the advice,I'll reproduce and fix it tonight, mayb add more unit test cases

9547 · 2020-09-18T15:15:31Z

@lucklove FINALLY all tests passed, thanks for your kindly help. One more thing, If I run make unit-test locally(even in the master branch), one panic occurred:

make unit-test

mkdir -p cover
TIUP_HOME=/home/.go/src/github.com/pingcap/tiup/tests/tiup GO111MODULE=on CGO_ENABLED=1 GO111MODULE=on CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go test -p 3 ./... -covermode=count -coverprofile cover/cov.unit-test.out
...
----------------------------------------------------------------------
PANIC: init_config_test.go:73: initConfigSuite.TestCheckConfig

... Panic: runtime error: invalid memory address or nil pointer dereference (PC=0x434745)

/usr/local/go/src/runtime/panic.go:969
  in gopanic
/usr/local/go/src/runtime/panic.go:212
  in panicmem
/usr/local/go/src/runtime/signal_unix.go:695
  in sigpanic
task.go:115
  in Context.GetExecutor
init_config.go:40
  in InitConfig.Execute
init_config_test.go:97
  in initConfigSuite.TestCheckConfig
/usr/local/go/src/reflect/value.go:321
  in Value.Call
/home/.go/pkg/mod/github.com/pingcap/[email protected]/check.go:850
  in suiteRunner.forkTest.func1
/home/.go/pkg/mod/github.com/pingcap/[email protected]/check.go:739
  in suiteRunner.forkCall.func1
/usr/local/go/src/runtime/asm_amd64.s:1373
  in goexit
OOPS: 0 passed, 1 PANICKED
--- FAIL: Test (0.00s)
FAIL
coverage: 0.9% of statements

lonng · 2020-09-19T01:17:26Z

pkg/repository/v1_repository.go

+
+		targetDir := filepath.Join(r.local.TargetRootDir(), localdata.ComponentParentDir, spec.ID, spec.Version)
+		target := filepath.Join(targetDir, versionItem.URL)
+		err = r.DownloadComponent(versionItem, target)


I think we should delete the target file when the component installed successfully.

Thanks for the advice, rm the target by default, and add an env named TIUP_KEEP_SOURCE_TARGET to keep the source target(maybe used for debugging propose).

lonng

Rest LGTM
Awesome works.

july2993 · 2020-09-19T07:29:20Z

r nil pointer dereference (PC=0x434745)

/usr/local/go/src/runtime/panic.go:969
  in gopanic
/usr/local/go/src/runtime/panic.go:212
  in panicmem
/usr/local/go/src/runtime/signal_unix.go:695
  in sigpanic
task.go:115
  in Context.GetExecutor
init_config.go:40
  in InitConfig.Execute
init_config_test.go:97
  in initConfigSuite.TestCheckConfig
/usr/local/go/src/reflect/value.go:321

This test case assumes you need to run failpoint-enable first 😔

lucklove · 2020-09-19T08:21:04Z

@lucklove FINALLY all tests passed, thanks for your kindly help. One more thing, If I run make unit-test locally(even in the master branch), one panic occurred:

make unit-test

mkdir -p cover
TIUP_HOME=/home/.go/src/github.com/pingcap/tiup/tests/tiup GO111MODULE=on CGO_ENABLED=1 GO111MODULE=on CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go test -p 3 ./... -covermode=count -coverprofile cover/cov.unit-test.out
...
----------------------------------------------------------------------
PANIC: init_config_test.go:73: initConfigSuite.TestCheckConfig

... Panic: runtime error: invalid memory address or nil pointer dereference (PC=0x434745)

/usr/local/go/src/runtime/panic.go:969
  in gopanic
/usr/local/go/src/runtime/panic.go:212
  in panicmem
/usr/local/go/src/runtime/signal_unix.go:695
  in sigpanic
task.go:115
  in Context.GetExecutor
init_config.go:40
  in InitConfig.Execute
init_config_test.go:97
  in initConfigSuite.TestCheckConfig
/usr/local/go/src/reflect/value.go:321
  in Value.Call
/home/.go/pkg/mod/github.com/pingcap/[email protected]/check.go:850
  in suiteRunner.forkTest.func1
/home/.go/pkg/mod/github.com/pingcap/[email protected]/check.go:739
  in suiteRunner.forkCall.func1
/usr/local/go/src/runtime/asm_amd64.s:1373
  in goexit
OOPS: 0 passed, 1 PANICKED
--- FAIL: Test (0.00s)
FAIL
coverage: 0.9% of statements

Use make test instead, it will make failpoint-enable && make unit-test

july2993 · 2020-09-21T03:52:36Z

pkg/repository/v1_repository.go

+// the component will be removed if hash is not correct
+func (r *V1Repository) DownloadComponent(item *v1manifest.VersionItem, target string) error {
+	targetDir := filepath.Dir(target)
+	err := r.mirror.Download(item.URL, targetDir)


seems we assume item.URL just a filename, but it may be /a/b/c/name, but we have no such manifest yet
not introduce by this pr, seems the master code do this.
@lucklove PTAL

9547 · 2020-09-21T23:17:47Z

@lucklove sorry to bother you again, In my local env, the tls related test case(integrate-cluster-scale/test_scale_tools_tls) failed randomly, the failed test log shows as below:

Starting component tidb
        Starting instance tidb 172.19.0.102:4000
        Starting instance tidb 172.19.0.101:4000
        Start tidb 172.19.0.102:4000 success
retry error: operation timed out after 2m0s
        tidb 172.19.0.101:4000 failed to start: timed out waiting for port 4000 to be started after 2m0s, please check the log of the instance

Error: failed to start tidb:    tidb 172.19.0.101:4000 failed to start: timed out waiting for port 4000 to be started after 2m0s, please check the log of the instance: timed out waiting for port 4000 to be started after 2m0s

Verbose debug logs has been written to /tiup-cluster/tests/tiup-cluster/logs/tiup-cluster-debug-2020-09-21-22-59-31.log.

Checking the TiDB 172.19.0.101:4000's failed log, shows the TiKV/TiDB can't connected to PD, seems connecting to the SSL certificate failure:

eg: log/tidb.log

[2020/09/21 22:57:31.230 +00:00] [FATAL] [misc.go:512] ["could not load cluster ssl"] [error="failed to append ca certs"] [stack="github.com/pingcap/tidb/util.initInternalClient\n\t/home/jenkins/agent/workspace/tidb_v4.0.4/go/src/github.com/pingcap/tidb/util/misc.go:512\nsync.(*Once).doSlow\n\t/usr/local/go/src/sync/once.go:66\nsync.(*Once).Do\n\t/usr/local/go/src/sync/once.go:57\ngithub.com/pingcap/tidb/util.InternalHTTPClient\n\t/home/jenkins/agent/workspace/tidb_v4.0.4/go/src/github.com/pingcap/tidb/util/misc.go:499\nmain.setupLog\n\t/home/jenkins/agent/workspace/tidb_v4.0.4/go/src/github.com/pingcap/tidb/tidb-server/main.go:622\nmain.main\n\t/home/jenkins/agent/workspace/tidb_v4.0.4/go/src/github.com/pingcap/tidb/tidb-server/main.go:176\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:203"]

lonng · 2020-09-21T23:58:42Z

@AstroProfundis PTAL

lonng

LGTM

lonng added the type/enhancement Categorizes issue or PR as related to an enhancement. label Sep 4, 2020

AstroProfundis requested a review from lucklove September 8, 2020 03:38

9547 force-pushed the feature/download-in-stream-mode branch 2 times, most recently from 73653b5 to 8c22ea8 Compare September 14, 2020 12:49

lucklove reviewed Sep 16, 2020

View reviewed changes

pkg/repository/v1_repository.go Show resolved Hide resolved

9547 force-pushed the feature/download-in-stream-mode branch from 3025d75 to 281f240 Compare September 16, 2020 23:12

lucklove closed this Sep 17, 2020

lucklove reopened this Sep 17, 2020

lucklove linked an issue Sep 17, 2020 that may be closed by this pull request

Reduce memory usage when downloading components #443

Closed

9547 force-pushed the feature/download-in-stream-mode branch from 360fafe to fe7e943 Compare September 18, 2020 14:55

lonng reviewed Sep 19, 2020

View reviewed changes

july2993 reviewed Sep 21, 2020

View reviewed changes

jsvisa added 13 commits September 21, 2020 22:49

repository,cluster: download target in stream mode

c4de5b4

repository: UpdateManifests use Download to repalce Fetch

651878a

pkg/repository: implement MockMirror.Download

4dfb1eb

gitignore: ignore failpoint files

1117b2c

pkg/repository: fix comment by make check

1d94ec4

repository: rm component if hash checksum no equal

2cb62d9

cluster: ensure mirror.Open after NewMirror

46e91bb

repository: move file if name not match

a4ac339

docker/control: killall is from package psmisc

c16fa2d

Fix: add mirror.Open after NewMirror

4e2138c

Chore(make): indent

0b6cf1f

localdata: add EnvNameKeepSourceTarget to keep source target or not

b23f865

repository: rm source target if expand

ac0bcff

9547 force-pushed the feature/download-in-stream-mode branch from 3cfbc85 to ac0bcff Compare September 21, 2020 14:49

lonng added 2 commits September 22, 2020 13:29

Merge branch 'master' into feature/download-in-stream-mode

18b9970

Merge branch 'master' into feature/download-in-stream-mode

0f329de

lonng approved these changes Sep 22, 2020

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 22, 2020

lonng merged commit 2d57d0b into pingcap:master Sep 22, 2020

9547 deleted the feature/download-in-stream-mode branch September 22, 2020 14:10

lucklove mentioned this pull request Sep 23, 2020

tiup playground oom #728

Closed

9547 mentioned this pull request Sep 26, 2020

Add more test case for target's spec.TargetDir #820

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/download in stream mode #755

Feature/download in stream mode #755

9547 commented Sep 3, 2020 •

edited

Loading

9547 commented Sep 3, 2020

lonng commented Sep 4, 2020

codecov-commenter commented Sep 13, 2020 •

edited

Loading

lucklove commented Sep 16, 2020 •

edited

Loading

lucklove commented Sep 16, 2020

9547 commented Sep 16, 2020

9547 commented Sep 16, 2020

lucklove commented Sep 17, 2020

9547 commented Sep 17, 2020

9547 commented Sep 17, 2020

lucklove commented Sep 18, 2020

lucklove commented Sep 18, 2020 •

edited

Loading

9547 commented Sep 18, 2020

9547 commented Sep 18, 2020 •

edited

Loading

lonng Sep 19, 2020

9547 Sep 21, 2020

lonng left a comment

july2993 commented Sep 19, 2020

lucklove commented Sep 19, 2020

july2993 Sep 21, 2020

9547 commented Sep 21, 2020

lonng commented Sep 21, 2020

lonng left a comment

Feature/download in stream mode #755

Feature/download in stream mode #755

Conversation

9547 commented Sep 3, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

9547 commented Sep 3, 2020

lonng commented Sep 4, 2020

codecov-commenter commented Sep 13, 2020 • edited Loading

Codecov Report

lucklove commented Sep 16, 2020 • edited Loading

lucklove commented Sep 16, 2020

9547 commented Sep 16, 2020

9547 commented Sep 16, 2020

lucklove commented Sep 17, 2020

9547 commented Sep 17, 2020

9547 commented Sep 17, 2020

lucklove commented Sep 18, 2020

lucklove commented Sep 18, 2020 • edited Loading

9547 commented Sep 18, 2020

9547 commented Sep 18, 2020 • edited Loading

lonng Sep 19, 2020

Choose a reason for hiding this comment

9547 Sep 21, 2020

Choose a reason for hiding this comment

lonng left a comment

Choose a reason for hiding this comment

july2993 commented Sep 19, 2020

lucklove commented Sep 19, 2020

july2993 Sep 21, 2020

Choose a reason for hiding this comment

9547 commented Sep 21, 2020

lonng commented Sep 21, 2020

lonng left a comment

Choose a reason for hiding this comment

9547 commented Sep 3, 2020 •

edited

Loading

codecov-commenter commented Sep 13, 2020 •

edited

Loading

lucklove commented Sep 16, 2020 •

edited

Loading

lucklove commented Sep 18, 2020 •

edited

Loading

9547 commented Sep 18, 2020 •

edited

Loading